Skip to content

fix(plugin-auth): let ImportProtocolLike type the admin import protocol's three members - #17489

Merged
os-sales merged 2 commits into
mainfrom
claude/issue-17422-import-protocol-any
Sep 10, 2026
Merged

fix(plugin-auth): let ImportProtocolLike type the admin import protocol's three members#17489
os-sales merged 2 commits into
mainfrom
claude/issue-17422-import-protocol-any

Conversation

@os-sales

@os-sales os-sales commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Fixes #17422

Clause-②: no

_(declaration line written by the domain:services review seat, not by the implementer. Check Changeset went RED on head fb659886 for the ABSENCE of this line, not for a wrong one: with no declaration and no needs:contract-review carrier, the gate resolves not-measured-material — a moved package graded patch is exactly what a yes would have refused. Declaring no resolves it as not-declared. Measured on THIS diff, ⛔ not predicted from the dispatch — the dispatch predicted no on the reasoning that deleting a parameter annotation only narrows, and that reasoning did NOT cover the new refusal this diff adds, so both limbs were re-measured:

limb A (accept set): unchanged. A findData call arriving without query was already rejected — by an incidental TypeError on a property read of undefined. It is now rejected by a named throw. Nothing is newly accepted and nothing is newly refused; the same call fails, legibly. ⛔ Not a widening.
limb B (public surface): unchanged. 0 added lines containing export in packages/** outside tests, against a control of 23 added source lines. The 1 added code: entry is INVALID_REQUEST, which is already registered in packages/spec/src/api/error-code-ledger.zod.ts — and @objectstack/plugin-auth is already listed as an owner of it (:550, inside the owner block opening at :536), so check:error-code-provenance needs neither a new owner-key row nor a PROVENANCE_WAIVERS entry. ⛔ No packages/spec edit is owed, and none is made.

no + a patch changeset is the correct and consistent pair here. ⛔ Do not delete this line; the gate reads the body, so removing it re-reds the PR.)_

What this closes

packages/plugins/plugin-auth/src/admin-import-users.ts is the only hand-written in-repo implementor of the runner's ImportProtocolLike, and it annotated all three required members args: any. An explicit parameter annotation wins over the contextual type, so the contract #16952 declared held every implementor except the one with the demonstrated history — the file whose frozen $filter read once degraded the import's duplicate probe into match-everything and updated the wrong users.

Three annotations removed, all members of the const protocol: ImportProtocolLike = { literal (positions re-derived by text, not from the card's line numbers):

member was now
findData async findData(args: any) async findData(args)
createData async createData(args: any) async createData(args)
updateData async updateData(args: any) async updateData(args)

The three OPTIONAL members of the interface — createManyData, insertManyData, validateData — are not implemented by this literal (grep -c for each: 0 implementations; createManyData's absence is deliberate and commented in place). So the class the card names is exactly three here, not "three plus".

Repo-wide sweep for other implementors: every other ImportProtocolLike literal is in packages/rest's own test doubles, and #16952 already annotated those FROM the exported declaration (Parameters[ImportProtocolLike['findData']] and siblings). The two production runImport call sites in rest-server.ts hand it the real DataProtocol service, not a hand-written literal.

Triage's fork, answered: MERELY UNTYPED — not a dialect divergence

Triage asked to stop and report if the errors revealed a genuinely different dialect. They do not. Every key this file reads is one the protocol declares:

  • findData reads args.object, args.query.where, args.query.limitFindDataRequest declares object and query; QuerySchema declares where and limit.
  • createData reads args.data — declared, required.
  • updateData reads args.data and args.id — both declared, both required.

The compiler reported two errors and both are about optionality, not about an undeclared key:

src/admin-import-users.ts(367,21): error TS18048: 'args.query' is possibly 'undefined'.
src/admin-import-users.ts(368,21): error TS18048: 'args.query' is possibly 'undefined'.

FindDataRequestSchema declares query optional (packages/spec/src/api/protocol.zod.ts:1865), while all three of the runner's findData dispatch sites always supply it (import-runner.ts:446, :489, :612, all through findArgsBase). So the file was reading the right key untypedly, and the contract's only complaint is that the file never wrote down what it does when the caller omits query.

Resolution — one change, no cast. findData now states the refusal instead of leaning on an incidental TypeError from a property read on undefined:

const query = args.query;
if (!query) {
  throw Object.assign(
    new Error('import-users: findData was called without a query — refusing to match every user'),
    { code: 'INVALID_REQUEST' },
  );
}
const where = query.where;
const limit = query.limit;

No as any, no @ts-expect-error, no !, no widened local alias, and deliberately no args.query ?? {} and no args.query?.where — both of those spell match-everything, which is the exact regression the file's own comment block is about. INVALID_REQUEST is already registered for @objectstack/plugin-auth in packages/spec/src/api/error-code-ledger.zod.ts:550, so no spec file is touched by this PR (⛔ packages/spec was the lane's red line) and pnpm check:error-code-casing stays green.

packages/rest/src/import-runner.ts was read only. Nothing in it needed to move.

Reverse verification — the green is a reading, not a dead check

Two legs plus a control, then restored by state.

leg tree pnpm --filter @objectstack/plugin-auth exec tsc --noEmit
1 — mutation parameter UNANNOTATED + probe reading the retired wire alias args.query?.$filter exit 1src/admin-import-users.ts(384,43): error TS2339: Property '$filter' does not exist on type 'QueryInput'.
2 — control the identical probe, with args: any restored exit 0
3 — restore git checkout HEAD -- path blob a371c6d7c7938833b7130040b98335c0cfc6656d equals the HEAD blob; git diff HEAD empty; git status --porcelain empty

Leg 2 is the point: the probe does not decide the colour — the annotation does. (The card cited TS2353; that came from an object-literal probe. This probe is a property READ, so the same live type answers TS2339. Direction as predicted; the code differs because the probe shape differs.)

Does a regression test for the $filter to match-everything history exist?

Behaviourally, yes — and it discriminates. Ablated findData back to the pre-#16950 read ((args.query as any)?.$filter ?? {}) and ran the sibling suite: 2 of 26 failed

  • upsert > matches by email …AssertionError: expected 2 to be 1 (with where empty the second row matches the existing user instead of creating one: the wrong-user update, reproduced)
  • upsert > matches by phone_number when enabled — the expect(find).toHaveBeenCalledWith(…, objectContaining({ where: … })) pin fails.

Restored by state (blob hash equal to HEAD, git diff HEAD empty).

For the opt-out itself, no — nothing could see it. Re-annotating all three members any and re-running: tsc --noEmit exit 0 and all 26 behavioural tests green. That is precisely how this hole stayed open under a green #16952. So this PR adds the missing guard as a source pin in the package's own test sibling (the same technique rest-server-canonical-query-ast.test.ts §1b uses for the declaration half). Ablated: with the three annotations restored, 3 of 30 fail — exactly the three new pins — and the other 27 stay green.

Verification

Working tree clean at fb6598867; every exit code captured before any pipe.

  • pnpm --filter @objectstack/plugin-auth build — exit 0, check-dts-emitted: 2/2 declared declaration file(s) present.
  • pnpm --filter @objectstack/plugin-auth typecheckexit 0 (tsc --noEmit + tsconfig.examples.json + check:test-typecheck; the test layer compiles under tsconfig.test.json with its shrink-only debt ledger unmoved: 10 files / 94 errors / 23 pinned signatures).
  • pnpm --filter @objectstack/plugin-auth testexit 0, 106 files / 2260 tests passed.
  • pnpm lint (eslint . --no-inline-config, the whole repo, not a narrowed set) — exit 0 in 1m53s at this same head.
  • Dependency closure built first: pnpm --filter '@objectstack/plugin-auth^...' build — exit 0, so tsc read a freshly built packages/rest/dist/index.d.ts carrying the narrowed interface (verified by grepping the three member signatures out of the emitted .d.ts), not a stale one.
  • scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack derived 61 families; all 61 run, 59 exit 0, 2 exit 3 = PREREQUISITE NOT MET = NOT MEASURED (check:dual-build-cjs-loads, check:type-check-debt — both refuse without a whole-workspace build, which is CI's Build Core / TypeScript Type Check, not a package-scoped local tier). --ran reconciliation with exit codes recorded: 61 derived, 59 run, 2 NOT-MEASURED, 0 UNRUN.
  • The five artifact-roster gates whose roster sits under a path in this diff were run rather than read as silent: check-changeset-fixed, check:auth-mount-ledger, check:authz-resolver, check:error-code-casing, check:filter-alias-parity — all exit 0.
  • Heavy runs went through scripts/pm/os-verify-lock.sh; every verdict read from its VERDICT command-exit line.

Acceptance notes

Observations from this file, deliberately not changed here and not filed (none meets the file-an-issue bar — no repro, no violated declared contract, no metadata-authoring trap):

  • IdentityImportEngine at src/admin-import-users.ts:89:91 (find(objectName: string, query?: any), update(…, data: any, options?: any), insert(…)) is a different, local, engine-shaped interface and not part of ImportProtocolLike. Left exactly as found, per the dispatch. Typing it against ObjectQL's real engine surface would be its own card; no PR or seat is in flight over it today.
  • createData / updateData still spell args?.data ?? {}. Both request types declare data required, so the fallback is now provably unreachable rather than merely unused — belt-and-braces of the family Prime Directive Add comprehensive test suite for Zod schema validation #12 names, but not a defect and not compiler-forced. The next PR touching this literal is the natural carrier.
  • engine.find(args.object, { where, limit, context: SYSTEM_CTX } as any) keeps its pre-existing as any on the OPTIONS bag (a consequence of the local interface above, not of the protocol). pnpm check:query-options-erasure is green over it.
  • The runner's docblock instruction ("Leave the parameter unannotated and let this declaration type it") is now enforced for this implementor only, by the pin added here. A future implementor in another package gets no such guard; a repo-wide check would be a separate piece of work, and nothing violates the contract today.

Generated by Claude Code

…bers

The `const protocol: ImportProtocolLike` literal in `admin-import-users.ts`
annotated all three required members `args: any`. An explicit parameter
annotation wins over the contextual type, so the one hand-written in-repo
implementor of the protocol was the one implementor NOT checked against the
request dialect the runner sends — the same file whose frozen `$filter` read
once degraded the duplicate probe into match-everything.

Deleting the three annotations lets `ImportProtocolLike` type the parameters.
`FindDataRequest` declares `query` optional, so `findData` now states its
refusal explicitly (already-ledgered `INVALID_REQUEST`) instead of relying on
an incidental TypeError from a property read on `undefined`. No `??` fallback,
no optional chaining: both spell match-everything.

Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW
Co-authored-by: Claude <noreply@anthropic.com>
…d changeset

The behavioural upsert tests already discriminate the CONSEQUENCE of the
historical read — ablated to `args.query?.$filter ?? {}`, two of the 26 go red
— but nothing in the repo can see the opt-out itself: re-annotating the
parameter `any` leaves every test and every gate green, which is exactly how
the hole this card closes stayed open. The new source pin is that guard.

Claude-Session: https://claude.ai/code/session_01ToDPcx9AESFubJkDiFMtKW
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/s documentation Improvements or additions to documentation tests tooling labels Sep 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-auth, touching 2 documentable anchor(s).

7 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/error-catalog.mdx (via INVALID_REQUEST (literal, a string literal in runAdminImportUsers))
  • content/docs/api/metadata-api.mdx (via INVALID_REQUEST (literal, a string literal in runAdminImportUsers))
  • content/docs/automation/webhooks.mdx (via INVALID_REQUEST (literal, a string literal in runAdminImportUsers))
  • content/docs/data-modeling/import-mappings.mdx (via INVALID_REQUEST (literal, a string literal in runAdminImportUsers))
  • content/docs/permissions/authentication.mdx (via INVALID_REQUEST (literal, a string literal in runAdminImportUsers))
  • content/docs/permissions/sso.mdx (via INVALID_REQUEST (literal, a string literal in runAdminImportUsers))
  • content/docs/ui/forms.mdx (via INVALID_REQUEST (literal, a string literal in runAdminImportUsers))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v17/17-0.mdx (via INVALID_REQUEST (literal, a string literal in runAdminImportUsers))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 13 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json fa23d69875d1d75b55afc485790fb3df3c9fb712packageMentionDocs.

Which tree this was computed on

This run read content/docs from 9adf5942d60a146be5654754a7f15ad3d86db4f7 — the merge of head fb659886748439fe7283763aa956d659864686ac into base fa23d69875d1d75b55afc485790fb3df3c9fb712, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 9adf5942d60a146be5654754a7f15ad3d86db4f7 && git checkout 9adf5942d60a146be5654754a7f15ad3d86db4f7
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin fa23d69875d1d75b55afc485790fb3df3c9fb712 fb659886748439fe7283763aa956d659864686ac && git checkout -B drift-repro fa23d69875d1d75b55afc485790fb3df3c9fb712 && git merge --no-ff fb659886748439fe7283763aa956d659864686ac

node scripts/docs-audit/affected-docs.mjs --json fa23d69875d1d75b55afc485790fb3df3c9fb712

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs fa23d69875d1d75b55afc485790fb3df3c9fb712 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Copy link
Copy Markdown
Collaborator Author

Docs Drift Check — answered per page, on the tree the bot read

Re-derived on 9adf5942d60a146be5654754a7f15ad3d86db4f7 (the merge of head fb659886748439fe7283763aa956d659864686ac into base fa23d69875d1d75b55afc485790fb3df3c9fb712), fetched into a ref of my own and read in a throwaway comparison worktree — not on a worktree cut from an older main. node scripts/docs-audit/affected-docs.mjs --json fa23d69875d1d75b55afc485790fb3df3c9fb712 there reproduced the advisory exactly: the same 8 pages, the same 2 anchors (1 symbol, 1 literal), computedOn.dirty: false.

The anchor is real, not incidental. This diff genuinely adds { code: 'INVALID_REQUEST' } on a new throw. Measured on the diff vs merge base: +1 throw and +1 code: entry across packages/** outside tests, and 0 exports added or removed.

But the emission is unreachable, and it is not an endpoint-level refusal. The const protocol: ImportProtocolLike literal is a function-local const inside runAdminImportUsers (admin-import-users.ts:351) — never exported, so runImport is its only caller. All four dispatch sites build the request through findArgsBase and always supply query (import-runner.ts:446, :489, :612, plus recheckByIds). And a throw out of findData is caught by the per-row catch at import-runner.ts:879 and folded by toFailedResult (:344) into a row verdictrows[].code inside an unchanged HTTP 200 envelope (admin-import-users.ts:610), never a top-level error.code. So no page documenting a 400 INVALID_REQUEST refusal can be falsified by it.

Per-page verdicts, with the falsifying shape named

page what its INVALID_REQUEST is about verdict
content/docs/api/error-catalog.mdx The /meta type-boundary section (:526:609): two refusals, both 400, told apart by message. Not falsified. The falsifying shape would be a claim that INVALID_REQUEST is emitted only at the /meta boundary, or an exhaustive emitter inventory. Neither exists — the section says in its own words 「It is not a complete inventory of /meta errors」 (:531), and the page's framing scopes it to the wire face, the codes a client can receive as error.code. This diff adds no wire-reachable code and no top-level error.code at all.
content/docs/api/metadata-api.mdx :13 and :73/meta route refusals, cross-linked to the section above. Not falsified. Different routes; no completeness claim about the code.
content/docs/automation/webhooks.mdx :551 — the codes the webhook redelivery endpoint answers. Not falsified. Different endpoint, different package path.
content/docs/data-modeling/import-mappings.mdx :222 — a whole-request rejection on POST /api/v1/data/:object/import (5,000-row ceiling, :272). Not falsified. This page documents the generic data import route, not POST /api/v1/auth/admin/import-users (500 rows, plugin-auth), and its INVALID_REQUEST row is under 「Rejections before any row is read … there is no per-row report」. Nothing in this diff touches that route or that stage. See the note below for a pre-existing observation about the same page.
content/docs/permissions/authentication.mdx :885 — the refusal table for POST /organization/add-member. Not falsified. Different endpoint, and it is that endpoint's own 400.
content/docs/permissions/sso.mdx :273 — the two SSO domain-verification calls refusing a missing providerId. Not falsified. Different endpoints.
content/docs/ui/forms.mdx :227 and :284 — the public form submit / lookup endpoints. Not falsified. Different endpoints.
content/docs/releases/v17/17-0.mdx :540 — the legacy-condition to standard-code mapping (bad_request maps to INVALID_REQUEST). Read, not edited. Not falsified. It is a vocabulary mapping table with no emitter claim. Release-owned; no change proposed.

The half the bot says it can never do — emitter-blind hand sweep

Its own note: a page stating a rule by its inputs shares no identifier with the emitter, so an emitter-only diff cannot list it. So I hand-checked the pages that would describe this endpoint's failure behaviour without naming INVALID_REQUEST or runAdminImportUsers. Naming them so nobody redoes it:

  • content/docs/permissions/authentication.mdx → "Bulk Import Users" (:951:995) — the one page that actually documents POST /api/v1/auth/admin/import-users. Read in full: it documents payload shapes, the four passwordPolicy behaviours, mode/matchBy, the 500-row ceiling and the Console wizard. It states no failure-code table and nothing about what the endpoint answers on a malformed request. Nothing to falsify. :1121 (the endpoint index line) likewise.
  • content/docs/data-modeling/import-mappings.mdx → "Per-row outcomes" (:227:243) — the only page enumerating per-row code values (NO_MATCH, BLANK_MATCH_KEY, AMBIGUOUS_MATCH, a field error code). It is scoped to the generic route, so this diff does not reach it.
  • content/docs/kernel/services-checklist.mdx:314 names admin-import-users.ts in a "ships in" table; :235 lists findData with its request/response types. Neither states failure behaviour, and no signature moved.
  • content/docs/releases/v14.mdx:113, content/docs/releases/v16.mdx:299 — release-owned, describe the endpoint's capabilities, not its codes. Read, not edited.
  • Repo-wide greps used, all over content/docs on the merge tree: import-users / importUsers / admin/import (4 files, all above), matchBy / "bulk import" (9 files, triaged), ImportProtocolLike / runImport / findData (5 hits, all about the DataProtocol service, none about this literal), IMPORT_ROW_FAILED (2 hits, both under the auto-generated content/docs/references/ tree, which is regenerated from the ledger and unaffected because no ledger entry was added).

One observation, explicitly not a falsification by this PR

content/docs/data-modeling/import-mappings.mdx:232:240 presents the per-row code table as an enumeration, and IMPORT_ROW_FAILED — the generic fallback toFailedResult has produced for any uncoded throw since long before this branch — is absent from it. That gap exists identically on the merge base and is not created, widened or reached by this diff. Recorded here so the next reader does not attribute it to this change; content/docs/** is domain:devx and nothing here was edited.

No page is falsified, so there is nothing to file and nothing under content/docs/** was touched.


Generated by Claude Code

@os-sales
os-sales marked this pull request as ready for review September 10, 2026 17:23
@os-sales
os-sales enabled auto-merge September 10, 2026 17:24
@os-sales
os-sales added this pull request to the merge queue Sep 10, 2026
Merged via the queue into main with commit 96684bb Sep 10, 2026
40 of 41 checks passed
@os-sales
os-sales deleted the claude/issue-17422-import-protocol-any branch September 10, 2026 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/s tests tooling

Projects

None yet

2 participants